Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

♻️ refactor: Response DTO 적용, request/response 분리 #9

Merged
merged 9 commits into from
Jan 9, 2025

Conversation

Jo-Minseok
Copy link
Collaborator

@Jo-Minseok Jo-Minseok commented Jan 8, 2025

🔨 테스크

Response DTO의 필요성

  • 코드를 볼 때, API 명세서가 있긴 하지만, 진짜 그 값을 반환하는지 확인하기 어려웠다.
  • 덕분에 런타임에 타입 오류가 있었던 부분을 잡을 수 있었다.
  • TS는 런타임에 타입을 잡는 게 아니라서 Redis에서 받은 값에 대한 타입이 잘 못 되어있었는데 우연찮게 코드가 돌아갔었다.

PR 룰

  • 무한 죄송 ㅠ.ㅠ
  • 코드 일관성 맞추는 건 따로 PR 다시 작성해서 작업하겠습니다.
  • 앞으로 작업 더 세분화하도록 해야겠군요...
    image

📋 작업 내용

[핵심 내용]

  • DTO 생성
  • DTO 적용하여 값 반환
  • FeedRecentRedis 타입 구현 recent.dto.ts
  • DTO 변환 함수들 이름 통일화
    • 일반 객체: toResponseDto
    • 배열 객체: toResponseDtoArray

@Jo-Minseok Jo-Minseok added the 🔨 Refactor 리팩토링 (구조 변경) label Jan 8, 2025
@Jo-Minseok Jo-Minseok self-assigned this Jan 8, 2025
Copy link
Collaborator

@CodeVac513 CodeVac513 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DTO 작업 외에도, 라인 수 줄이는 작업도 중간 중간 하셨더라구요. 쉽지 않았을텐데 정말 수고 많으셨습니다.
다 합리적으로 잘 리팩토링이 된 것 같습니다.

P3) 하나 토의 해보고 싶은 내용이 있는데요, toResponseDtoArray 관련입니다.

현재 어떤 DTO는 Array 메서드가 구현되어 있고, 어떤 DTO는 없는 상태입니다.

물론 없는 DTO는 그 DTO에게 필요가 없기 때문에 구현하지 않은 것을 우리는 알고 있습니다. (예를 들면 FeedPaginationResponseDto)
이런 DTO들은 이미 데이터가 배열 형태로 삽입되기에 굳이 DTO가 Array형태를 가질 필요가 없죠.

하지만 코드를 처음 보는 사람 입장에서는 Dto들을 사용하다가
어, 이 코드는 왜 Array 변환이 없지? 혹시 메서드가 누락된건가? 아니면 Array로 사용하는 DTO가 아닌가? 등등의 고민을 하게 될 것 같습니다.
이런 사항에 대해서는 어떻게 처리할 수 있을지 다른 분들 의견이 궁금하네요.
@asn6878 @Jo-Minseok
(제가 규모가 작은 프로젝트에서 너무 유지보수나 미래를 생각하나 싶기도 합니다 ㅋㅋ...)

@Jo-Minseok Jo-Minseok changed the title Refactor/response-dto ♻️ refactor: Response DTO 적용, request/response 분리 Jan 8, 2025
@Jo-Minseok
Copy link
Collaborator Author

Jo-Minseok commented Jan 8, 2025

@CodeVac513 일반 커멘트라 답글을 못 적어서 따로 커멘트 남깁니당.
코드 라인 수 이미 망했지만 그나마 지키려고 부랴부랴 올렸습니다.
그래서 PR에 코드 일관성 맞추는 건 따로 PR 다시 작성해서 작업하겠습니다. 라고 적어놨는데, 이 작업할 때 통일하려고 했어용. 이 작업 할 때,

  • Controller 계층에서 어떤 건 data 변수 생성해서 반환하고, id 변수 생성하고 반환하는데, 가독성상 그렇게 도움이 되는 건 아닌 것 같아 메모리를 위해 없앨 예정입니다.
  • 성윤님의 Repository 계층 재사용성에 대한 생각으로 Repository 계층 인자들을 Object가 아닌, 일반 매개 변수로 받을 수 있게 통일화 시킬 예정입니다.
  • 몇몇 Controller에서 Param을 DTO를 사용하는 곳이 있고 일반 변수를 사용하는 곳이 있어 DTO로 통일할 예정입니다.
  • 변수 이름 result, item 처럼 추상화 되어 있는 것들 이름 다 바꿀 예정입니다.
  • DTO에 사용하지는 않지만 일관성을 위해 toResponseDtoArray를 만들어 둘 예정입니다. 나중에 사용할 수 있을 것 같고 통일성을 위해서라면 하는 게 맞다고 생각합니다.

@asn6878
Copy link
Member

asn6878 commented Jan 9, 2025

@CodeVac513 @Jo-Minseok
저는 불필요한 toResponseDtoArray는 만들 필요가 없다는 생각인데요.

일관성이야기를 해주셨는데, 일관성이 왜 필요한가? 를 고민해 보자, 개발자가 어떤 함수나 모듈을 개발할때 "기존에 작성된 코드를 보고 한눈에 작성할 코드의 형태를 파악 할 수 있게 하기 위해서" 라는 판단을 내렸습니다.

쉽게 예를 들어보자면
아, 이번에 만든 API DTO 작성 해야되네. 다른 DTO들은 어떻게 써져 있지?
메서드 네이밍은 이렇게하고 리턴 값은 이런 형태로 쓰는구나~

위의 과정을 빠르게 가능케 하는 것일관성의 주 목적이라고 생각되는데요.

허나 toResponseDtoArray의 필요성을 개발자가 헷갈릴 일이 있을까요? 저는 아니라고 생각합니다. 필요성은 개발자가 개발을 진행하며 판단하는 것이고, 이렇게 혼란의 여지 없이 판단 할 수 있는 문제는 일관성과는 거리가 멀다고 생각해요.

이렇게 일관성의 이점을 챙기지 못한 상태로 단순히 '다른 메서드들도 썼으니까 얘도 써야지' 라는 생각으로 쓰는 것은 코드의 양만 늘릴 것 같다는 생각입니다.

Copy link
Member

@asn6878 asn6878 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

확인 했습니다~

앞으로 DTO 쓸때 헷갈릴 일은 없겠네요 😄

@Jo-Minseok
Copy link
Collaborator Author

@asn6878 그럼 필요한 곳에서만 toResponseDtoArray 함수를 생성하도록 하겠습니다! 저는 이 방법도 좋다고 생각합니다.

@Jo-Minseok Jo-Minseok merged commit ac64d83 into main Jan 9, 2025
1 check passed
@Jo-Minseok Jo-Minseok deleted the refactor/response-dto branch January 9, 2025 08:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔨 Refactor 리팩토링 (구조 변경)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants